Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(parse/json): change fields in JsonParserSettings to Option #3272

Merged
merged 1 commit into from
Jun 25, 2024
Merged

refactor(parse/json): change fields in JsonParserSettings to Option #3272

merged 1 commit into from
Jun 25, 2024
+36 −24

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Jun 24, 2024

Summary

This changes the JSON parser options to use Option<bool> for better semantics when dealing with default values and fallbacks.

as discussed here: #3260 (comment)

Test Plan

CI should pass

@github-actions github-actions bot added A-Project Area: project A-Parser Area: parser L-JSON Language: JSON and super languages labels Jun 24, 2024
This was referenced Jun 24, 2024
Merged
Merged
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jun 24, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3272 will improve performances by 6.73%

Comparing dyc3:06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ (a2574da) with main (a3c2b18)

Summary

⚡ 1 improvements
✅ 89 untouched benchmarks

Benchmarks breakdown

Benchmark main dyc3:06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ Change
⚡ jquery.min.js[cached] 28.7 ms 26.9 ms +6.73%

@dyc3 dyc3 force-pushed the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch from adf5158 to 12c6730 Compare June 24, 2024 12:13
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch from 12c6730 to 5d1aab5 Compare June 24, 2024 13:09
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch from 5d1aab5 to a2574da Compare June 24, 2024 18:44
@dyc3 dyc3 requested a review from Sec-ant June 24, 2024 23:54
Sec-ant
Sec-ant approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@Sec-ant Sec-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix! Could you also remove JsonParserOptions from the PR title?

@dyc3 dyc3 changed the title refactor(parse/json): change fields in JsonParserOptions and JsonParserSettings to Option refactor(parse/json): change fields in JsonParserSettings to Option Jun 25, 2024
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 25, 2024

Done

@Sec-ant Sec-ant merged commit 91522a5 into biomejs:main Jun 25, 2024
13 checks passed
@dyc3 dyc3 deleted the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch June 26, 2024 13:20
@dyc3 dyc3 mentioned this pull request Jun 26, 2024
Open
@Sec-ant Sec-ant mentioned this pull request Jun 29, 2024
Merged
@ramzimalhas ramzimalhas mentioned this pull request Aug 21, 2024
Open
This was referenced Aug 28, 2024
Open
Open
@Ramyromel Ramyromel mentioned this pull request Sep 9, 2024
Open
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
@Ramyromel Ramyromel mentioned this pull request Sep 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sec-ant Sec-ant Sec-ant approved these changes

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
A-Changelog Area: changelog A-Parser Area: parser A-Project Area: project L-JSON Language: JSON and super languages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dyc3 @ematipico @Sec-ant @Conaclos